Skip to content

Mentioned Debian explicitly #9899

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jun 18, 2018
Merged

Conversation

ThomasLandauer
Copy link
Contributor

Question: The next sentence ("That's why...") doesn't make sense to me! If Debian thinks it's better to do it with its own cronjob, why is Symfony overwriting it?? => Suggestion: Symfony overwrites this value to 1, because ...

Question: The next sentence ("That's why...") doesn't make sense to me! If Debian thinks it's better to do it with its own cronjob, **why** is Symfony overwriting it?? => Suggestion: Symfony overwrites this value to ``1``, because ...
@javiereguiluz
Copy link
Member

I'm not sure about this change. The reason is that we're always hesitant to include too specific info like mentioning Debian as one of the OS that do this. The problem of doing that is that it forces us to check every current and future version of Debian to see if that statement is still true. But let's wait to read more opinions about this. Thanks!

@ThomasLandauer
Copy link
Contributor Author

I committed this PR primarily cause I had this message several times in my nginx error log, and it was hard to find out what the reason is:

PHP message: PHP Notice: SessionHandler::gc(): ps_files_cleanup_dir: opendir(/var/lib/php/sessions) failed: Permission denied (13)

So what I'm really suggesting would be two things:

  • Include this error message on the page too, to have people find it when they search for it. I could do this.

  • Explain why Symfony is doing this at all! I can't do this, since I don't know :-(

The "Debian" question is less important. And I certainly see your point. What about something like "some versions of Debian"? Making an exception for Debian is justified, I would say, since it's (a) (probably) the most widely used OS on webservers, and (b) many other Linux distros depend on it.

But, again, the other two points are far more important!

@javiereguiluz
Copy link
Member

I think we need help from someone who knows this well (@Tobion, @stof, @nicolas-grekas, ...) so they can explain to us why does Symfony do this. Thanks!

@javiereguiluz
Copy link
Member

@nicolas-grekas thanks for reviewing! We'd also need your help to understand this:

Explain why Symfony is doing this at all! I can't do this, since I don't know :-(

Symfony knows better than Debian and overrides this setting silently? Thanks!

@stof
Copy link
Member

stof commented Jun 12, 2018

@javiereguiluz as soon as you configure a session save path in the Symfony config (rather than using the one from php.ini), relying on the Debian cron job does not work at all (as the cron job will clean a different location than the one used when saving).

@javiereguiluz javiereguiluz added this to the 2.8 milestone Jun 18, 2018
@javiereguiluz
Copy link
Member

Thanks Thomas.

@javiereguiluz javiereguiluz merged commit 67d2f87 into symfony:2.8 Jun 18, 2018
javiereguiluz added a commit that referenced this pull request Jun 18, 2018
This PR was merged into the 2.8 branch.

Discussion
----------

Mentioned Debian explicitly

Question: The next sentence ("That's why...") doesn't make sense to me! If Debian thinks it's better to do it with its own cronjob, **why** is Symfony overwriting it?? => Suggestion: Symfony overwrites this value to ``1``, because ...

Commits
-------

67d2f87 Mentioned Debian explicitly
@ThomasLandauer ThomasLandauer deleted the patch-12 branch June 24, 2018 21:05
@ThomasLandauer
Copy link
Contributor Author

@stof Wouldn't it be better to include a comment inside the yaml file (something like "If you change this, make sure to...")?

I mean: The current setting produces error log entries for many people. That's not good ;-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants